Skip to content

spec: consolidate xxxProbe calls into Identity.Probe #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

jdef
Copy link
Member

@jdef jdef commented Feb 10, 2018

  • Remove ControllerProbe RPC.
  • Remove NodeController RPC.
  • Introduce Identity.Probe RPC, clarify its intent.

@jdef
Copy link
Member Author

jdef commented Feb 16, 2018

@jdef
Copy link
Member Author

jdef commented Feb 16, 2018

@saad-ali @jieyu @cpuguy83 @julian-hj PTAL

Copy link
Contributor

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

spec.md Outdated

A Plugin MUST implement this RPC call.
The primary utility of the Probe RPC is deployment verification: this can be a one time or periodic check to ensure that a plugin instance is healthy and has all dependencies available.
This information can be used, for example, to monitor the health of the plugin and redeploy the plugin (or take other automated measures) when it becomes unhealthy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deployment verification and checking of dependencies is something I would lump in to "initialization checks" and not the responsibility of Probe. By implying that it is, we implicitly expect a Probe call to happen around deployment time, and it may not happen until sometime afterwards.

I would suggest rewording this to be as generic as possible: The primary utility of the Probe RPC is to verify the plugin is in a healthy state. If an unhealthy state is reported, via a non-success response, the CO MAY take action (such as restarting the plugin container) to try and bring the plugin to a healthy state.

spec.md Outdated
@@ -1457,6 +1443,7 @@ Supervised plugins MAY be isolated and/or resource-bounded.
##### Available Services

* Plugin Packages MAY support all or a subset of CSI services; service combinations MAY be configurable at runtime by the Plugin Supervisor.
* A plugin must know the "mode" in which it's operating: mode of operation is NOT discoverable via interaction with the CO via the CSI specification.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: A plugin must know the "mode" in which it's operating (e.g. node, controller, or both). This specification does not dictate the mechanism by which mode of operation must be discovered, leaving it to the SP to decide.

@jdef
Copy link
Member Author

jdef commented Feb 17, 2018 via email

Copy link
Member

@jieyu jieyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo @saad-ali's suggestion. The rest looks good to me.

James DeFelice added 3 commits February 21, 2018 17:25
* Remove ControllerProbe RPC.
* Remove NodeController RPC.
* Introduce Identity.Probe RPC, clarify its intent.
@jdef jdef force-pushed the consolidate_probe_rpcs branch from 78b5ea6 to 948e2ec Compare February 21, 2018 17:42
@jdef
Copy link
Member Author

jdef commented Feb 21, 2018

incorporated latest feedback, rebased to master

Copy link
Contributor

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@saad-ali
Copy link
Member

LGTM. Merging.

@saad-ali saad-ali merged commit f36159a into container-storage-interface:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify Identity Service vs Probe request
4 participants